Conversation
| @Test | ||
| public void serverMetricsShouldRecordContextWithBaggage() { | ||
| // Mocks | ||
| DoubleHistogram serverCallDurationCounter = mock(DoubleHistogram.class); |
There was a problem hiding this comment.
It is only safe to mock classes under your control. Mocks break API guarantees and can do horrible things. Those can be addressed when you change your own class and it breaks a test. But if you don't own the class, you can't control it.
| Collection<String> optionalLabels, List<OpenTelemetryPlugin> plugins, | ||
| @Nullable TargetFilter targetAttributeFilter) { | ||
| ContextPropagators contextPropagators, | ||
| @Nullable TargetFilter targetAttributeFilter) { |
| streamPlugins = Collections.unmodifiableList(streamPluginsMutable); | ||
| } | ||
| return new ServerTracer(OpenTelemetryMetricsModule.this, fullMethodName, streamPlugins); | ||
| Context context = contextPropagators.getTextMapPropagator().extract( |
There was a problem hiding this comment.
This is being done by the OpenTelemetryTracingModule and is being propagated via filterContext(), specifically so it can be used by the metrics module. If we want the full context, propagate that instead. I don't understand the goal here (and there's no description about why we're doing this in the PR description or a comment from what I can tell). And I'm doubly confused by using this context directly half the time and copying BAGGAGE_KEY into it other times.
opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java
Show resolved
Hide resolved
|
|
||
| void recordFinishedAttempt() { | ||
| Context otelContext = otelContextWithBaggage(); | ||
| Context otelContext = otelContextWithBaggage(BAGGAGE_KEY.get()); |
There was a problem hiding this comment.
There is no guaranteed Context as this point. I think this is only working because you're using in-process, which calls the StatsTraceContext on the calling thread. If the test used Netty instead, I suspect the baggage would no longer be found.
2 participants
No description provided.